Skip to content

Specify element data via element type#600

Open
isteinbrecher wants to merge 1 commit intobeamme-py:mainfrom
isteinbrecher:block_element_data
Open

Specify element data via element type#600
isteinbrecher wants to merge 1 commit intobeamme-py:mainfrom
isteinbrecher:block_element_data

Conversation

@isteinbrecher
Copy link
Copy Markdown
Collaborator

The first step towards #594 is that we gather similar elements in element blocks.

An element for 4C is basically defined by its connectivity and element data. Until now, we store the element data in each element instance. A better approach would be to store the element data in the element type. This would allow us to consider all elements of the same type (and material) to be of the same element block.

This PR proposes to switch the element data into the element type. This is actually more or less straightforward, as we already did something like that for the beam elements, now we also do this for NURBS and solid elements.

@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

@davidrudlstorfer I am in no hurry merging this, but I would be curious about your opinion.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors element data storage for solid and NURBS elements to match the pattern already used for beam elements. Instead of storing element data in each element instance, the data is now stored in the element type itself via a class-level four_c_element_data attribute. This architectural change is the first step towards organizing elements into element blocks (issue #594), where elements of the same type and material can be grouped together.

Changes:

  • Introduced get_four_c_solid() function to create NURBS element types with element data stored at the class level
  • Updated NURBS mesh creation functions to accept element types instead of determining types internally
  • Modified element dumping and importing logic to work with class-level element data
  • Refactored input file mappings to use node count instead of element class for type string lookup

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/beamme/four_c/element_solid.py New file implementing get_four_c_solid() to create NURBS element types with class-level element data
src/beamme/mesh_creation_functions/nurbs_generic.py Updated to accept element types as parameters and removed internal type selection logic
src/beamme/four_c/input_file_dump_item.py Modified to access element data from class-level four_c_element_data attribute
src/beamme/four_c/input_file_mappings.py Refactored mappings to use node count instead of element class, reorganized beam type mappings
src/beamme/four_c/model_importer.py Added create_solid_element_type() to create element types with class-level data during import
src/beamme/four_c/solid_shell_thickness_direction.py Updated to use getattr() for safe access to class-level element data
src/beamme/four_c/element_beam.py Updated to use renamed beam_type_to_four_c_type mapping
src/beamme/four_c/element_rigid_sphere.py Corrected docstring from "volume elements" to "solid elements"
tests/integration/test_integration_mesh_creation_functions_nurbs.py Updated tests to create element types via get_four_c_solid() before passing to mesh creation functions
tests/integration/test_integration_four_c.py Updated test to create element type before adding NURBS to mesh
tests/beamme/mesh_creation_functions/test_beamme_mesh_creation_functions_nurbs_generic.py Updated test to create element type before adding NURBS to mesh

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Collaborator

@davidrudlstorfer davidrudlstorfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all I am super sorry for the long time to review this. Due to the FE exam I was busy the last weeks.

The overall PR looks really good and the code simplifies in a lot of places. More or less I only have a few questions/unclarity regarding functionality.

Comment on lines +76 to +79
return imported_element_hash_to_solid_type.setdefault(
imported_element_hash,
type("FourCSolidElementType", (base_type,), {"four_c_element_data": data}),
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not fully sold on this hashing thing here. Why do we need to hash the element, and on which "data" do we actually hash it? What could be the reason that we read one element 2 times? Maybe you could explain that to me:)

material=None,
data: dict | None = None,
data: dict
| None = None, # TODO: Check if this is really needed! Or unify this with how solid element data is processed
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One ToDo

*,
material=None,
data: dict | None = None,
data: dict | None = None, # TODO: Decide what todo about this
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And second ToDo

@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

@davidrudlstorfer thanks for the review, I hope I will be able to continue the work on this in the next weeks.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

src/beamme/four_c/element_rigid_sphere.py:33

  • With the new dumping logic, solid element types are expected to expose four_c_element_data (and rigid spheres appear to be handled via type == "RIGIDSPHERE"). SolidRigidSphere currently doesn't define four_c_element_data, so dumping/importing rigid spheres will fail. Add a class attribute like four_c_element_data = {"type": "RIGIDSPHERE"} (and ensure material handling stays consistent with the new get_four_c_element_data() rules).
    src/beamme/mesh_creation_functions/nurbs_generic.py:97
  • add_*_nurbs_to_mesh() still accepts a data argument and forwards it into the created element instance (data=data). But dumping now explicitly rejects any per-element element.data entries and requires all element data to live on the element type (four_c_element_data). This API mismatch will lead to runtime failures when callers pass data. Consider removing/deprecating the data parameter, or merging it into element_type.four_c_element_data (or a derived type) instead of storing it on the instance.
def add_splinepy_nurbs_to_mesh(
    mesh: _Mesh,
    element_type: type,
    splinepy_obj,
    *,
    material=None,
    data: dict
    | None = None,  # TODO: Check if this is really needed! Or unify this with how solid element data is processed
) -> _GeometryName:
    """Add a splinepy NURBS to the mesh.

    Args:
        mesh: Mesh that the created NURBS geometry will be added to.
        element_type: The type of element to be created.
        splinepy_obj (splinepy object): NURBS geometry created using splinepy.
        material (Material): Material for this geometry.
        data: General element data, e.g., material, formulation, ...

    Returns:
        GeometryName:
            Set with the control points that form the topology of the mesh.

            For a surface, the following information is stored:
                Vertices: 'vertex_u_min_v_min', 'vertex_u_max_v_min', 'vertex_u_min_v_max', 'vertex_u_max_v_max'
                Edges: 'line_v_min', 'line_u_max', 'line_v_max', 'line_u_min'
                Surface: 'surf'

            For a volume, the following information is stored:
                Vertices: 'vertex_u_min_v_min_w_min', 'vertex_u_max_v_min_w_min', 'vertex_u_min_v_max_w_min', 'vertex_u_max_v_max_w_min',
                        'vertex_u_min_v_min_w_max', 'vertex_u_max_v_min_w_max', 'vertex_u_min_v_max_w_max', 'vertex_u_max_v_max_w_max'
                Edges: 'line_v_min_w_min', 'line_u_max_w_min', 'line_v_max_w_min', 'line_u_min_w_min',
                    'line_u_min_v_min', 'line_u_max_v_min', 'line_u_min_v_max', 'line_u_max_v_max'
                    'line_v_min_w_max', 'line_u_max_w_max', 'line_v_max_w_max', 'line_u_min_w_max'
                Surfaces: 'surf_w_min', 'surf_w_max', 'surf_v_min', 'surf_v_max', 'surf_v_max', 'surf_u_min'
                Volume: 'vol'
    """

    # Make sure that the control points are 3D
    nurbs_cp_dim = splinepy_obj.control_points.shape[1]
    if not nurbs_cp_dim == 3:
        raise ValueError(f"Invalid control point dimension: {nurbs_cp_dim}")

    # Make sure the material is in the mesh
    mesh.add_material(material)

    # Fill control points
    control_points = [
        _ControlPoint(coord, weight[0])
        for coord, weight in zip(
            _np.asarray(splinepy_obj.control_points), _np.asarray(splinepy_obj.weights)
        )
    ]

    # Fill element
    element = element_type(
        [_np.asarray(knot_vector) for knot_vector in splinepy_obj.knot_vectors],
        _np.asarray(splinepy_obj.degrees),
        nodes=control_points,
        material=material,
        data=data,
    )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

I am not fully sold on this hashing thing here. Why do we need to hash the element, and on which "data" do we actually hash it? What could be the reason that we read one element 2 times? Maybe you could explain that to me:)

@davidrudlstorfer we actually don't hash the element here, but the element type and material, i.e., the things that are common for all elements in an element block. If we want to switch to more efficient data structures, we have to group all elements that have the same element formulation, parameters and material in a block. For example, all SR elements with a certain cross-section. KL elements would be in a different block.

This grouping is achieved by hashing the element type data, e.g., element formulation, number of nodes per element, integration scheme, ..., and the material together. So elements with the same hash belong to the same block in the 4C input file.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/beamme/utils/data_structures.py:39

  • dict_to_tuple_converter returns tuples that may still contain unhashable values (e.g., lists like the solid element description key GP: [3, 3]). This will raise TypeError: unhashable type: 'list' when used in hash((base_type, dict_to_tuple_converter(element_data))) during import. Consider converting lists/tuples/sets recursively (e.g., to tuples) and optionally handling other common containers (like numpy arrays) to guarantee the result is fully hashable.
    src/beamme/four_c/element_rigid_sphere.py:25
  • With the new element dumping logic (get_four_c_element_data requires type(element).four_c_element_data), SolidRigidSphere currently lacks the required four_c_element_data class attribute, so rigid sphere export will raise a ValueError. Define four_c_element_data on this class (at minimum including {"type": "RIGIDSPHERE"}) so dumping works and the material-optional rigid-sphere path can trigger.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Collaborator

@davidrudlstorfer davidrudlstorfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments

@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:10 — with GitHub Actions Inactive
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:17 — with GitHub Actions Inactive
@isteinbrecher isteinbrecher temporarily deployed to cubit_secrets_trusted March 30, 2026 13:33 — with GitHub Actions Inactive
@isteinbrecher
Copy link
Copy Markdown
Collaborator Author

@davidrudlstorfer this should now be ready for review. I changed the hashing of the element block data as discussed, now we only compare dictionaries.

With this PR the data structures of the elements now closely resemble the ones needed for binary output, which should make #594 a lot easier.

I am not sure, but maybe the current changes degrade performance a little bit - for legacy output. However, once we switch to optional binary output, performance is only relevant for that case anyways, since that should be a lot faster than the current output mechanism.

@isteinbrecher isteinbrecher deployed to cubit_secrets_trusted March 30, 2026 13:39 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants